-
Notifications
You must be signed in to change notification settings - Fork 136
[WOOMOB-1068] Add initial implementation of Local Catalog Settings #14717
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WOOMOB-1068] Add initial implementation of Local Catalog Settings #14717
Conversation
-woo-poslocal-catalog-implement-sync-section-in-settings # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/woopos/util/format/WooPosDateFormatter.kt # WooCommerce/src/main/res/values/strings.xml # WooCommerce/src/test/kotlin/com/woocommerce/android/ui/woopos/util/format/WooPosDateFormatterTest.kt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements the initial version of Local Catalog Settings for WooPos, introducing a new settings category that manages local catalog data and synchronization preferences.
- Adds a new "Catalog" settings category with status information, cellular data toggle, and manual refresh functionality
- Implements feature flag-based visibility to show/hide the Local Catalog settings
- Integrates analytics tracking for the new Local Catalog settings section
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| WooCommerce/src/main/res/values/strings.xml | Adds string resources for Local Catalog settings UI elements |
| WooPosAnalyticsEvent.kt | Adds analytics event for Local Catalog section tapping |
| WooPosSettingsLocalCatalogViewModel.kt | Implements ViewModel with catalog status loading and sync functionality |
| WooPosSettingsLocalCatalogState.kt | Defines state data classes for catalog status and UI state |
| WooPosSettingsLocalCatalogScreen.kt | Creates Compose UI for Local Catalog settings with status, toggle, and refresh sections |
| WooPosSettingsDetailPaneScreen.kt | Integrates Local Catalog screen into settings navigation |
| WooPosSettingsCategoriesViewModel.kt | Adds feature flag logic to conditionally show Local Catalog category |
| WooPosSettingsCategoriesState.kt | Adds LOCAL_CATALOG enum entry with navigation destination |
| WooPosSettingsViewModel.kt | Adds analytics tracking for Local Catalog category selection |
| WooPosSettingsState.kt | Defines navigation destination for Local Catalog settings |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
...merce/android/ui/woopos/settings/details/localcatalog/WooPosSettingsLocalCatalogViewModel.kt
Show resolved
Hide resolved
...merce/android/ui/woopos/settings/details/localcatalog/WooPosSettingsLocalCatalogViewModel.kt
Show resolved
Hide resolved
...merce/android/ui/woopos/settings/details/localcatalog/WooPosSettingsLocalCatalogViewModel.kt
Outdated
Show resolved
Hide resolved
...commerce/android/ui/woopos/settings/details/localcatalog/WooPosSettingsLocalCatalogScreen.kt
Show resolved
Hide resolved
Generated by 🚫 Danger |
📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.
|
|
📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.
|
| @@ -0,0 +1,14 @@ | |||
| package com.woocommerce.android.ui.woopos.settings.details.localcatalog | |||
|
|
|||
| data class WooPosSettingsLocalCatalogState( | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this state declaration is a bit ambiguous:
- Nullable catalogStatus - when it's
null, does that mean loading or error - not clear - Overlapping flags - you could have
isLoadingandisRefreshingbothtrueat the same time, which is can be correct, but I am not sure. When you hit refresh I'd assume we may want to show catalog status "loading" too?
Maybe something like that?
data class WooPosSettingsLocalCatalogState(
val catalogStatus: CatalogStatus = CatalogStatus.Loading,
val allowCellularDataUpdate: Boolean = false,
) {
sealed class CatalogStatus {
data class Available(
val catalogSize: String,
val lastUpdate: String,
val lastFullUpdate: String
) : CatalogStatus()
object Loading : CatalogStatus()
}
}
And when it's "loading" maybe to not show "refresh button" at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I'd just suggest thinking about the state declaration, making sure that it cannot be in an ambiguous state, and checking this in compile time. So ideally, to avoid flags and the nullables there.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #14717 +/- ##
============================================
- Coverage 38.37% 38.36% -0.01%
- Complexity 9855 9871 +16
============================================
Files 2099 2102 +3
Lines 117044 117132 +88
Branches 15657 15670 +13
============================================
+ Hits 44913 44938 +25
- Misses 67964 68026 +62
- Partials 4167 4168 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
#WOOMOB-1068
Description
This PR implements a first version of the Local Catalog settings.
Unit tests - I skipped unit tests for the VM, since the logic is just dummy. I'll add them in the follow up PR.
Question - currently, the full refresh is performed within VM scope. I'm wondering whether we want to keep it like this or if we want the button to schedule an immediate background sync instead. Wdyt?
Testing information
Verify the Catalog section is not visible when the Local Catalog FF is turned off.
The tests that have been performed
The above
Images/gif
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.